[APS-19076][APS-19077][APS-19078] fix: security hardening — env-var allowlist, workflow SHA pinning, report HTML sanitization#85
Open
Jimesh-browserstack wants to merge 3 commits intomasterfrom
Conversation
- APS-19076 (CVSS 9.3, env-var injection): allowlist env-var names
exported from the BrowserStack rerun API in setBStackRerunEnvVars.
Adds ALLOWED_RERUN_ENV_VARS (BROWSERSTACK_RERUN, BROWSERSTACK_RERUN_TESTS,
BROWSERSTACK_BUILD_NAME) to setup-env/config/constants.js; filtered
loop in setup-env/src/actionInput/index.js calls core.exportVariable
only for allowlisted keys and core.warning for everything else.
Updates existing test, adds negative-case test, rebuilds dist.
- APS-19077 (CVSS 8.7, supply chain + token scope): pins
actions/setup-node@v4.4.0 and actions/checkout@v4.2.2 by SHA in both
.github/workflows/setup-env.yml and setup-local.yml; adds top-level
permissions: { contents: read } block to give the GITHUB_TOKEN least
privilege (these workflows only run unit tests).
- APS-19078 (CVSS 7.6, stored XSS via report HTML): adds sanitize-html
to browserstack-report-action; sanitizes basicHtml/richHtml in
ReportProcessor.js with a strict allowlist (no inline event handlers,
no javascript: URLs); strips JS hooks from richCss with sanitizeCss();
injects CSP meta (script-src 'none'; default-src 'none') into the
artifact HTML head in UploadFileForArtifact.js as defense-in-depth;
rebuilds dist.
Tests: setup-env 37 passing (was 36, +1 negative-case); report-action
18 passing (no regression). Standalone XSS-strip sanity covers six
payloads including <img src=x onerror=alert(1)>, <script>, javascript:
URLs, <svg onload>, <iframe javascript:>; all stripped, safe HTML
preserved.
Resolves: APS-19076, APS-19077, APS-19078
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Integration test caught a click-to-XSS bypass: <a href="data:text/html,<script>...</script>"> survived because data: was allowed on all elements. Restrict to <img> for inline screenshots; block on <a>/<iframe>/etc.
CodeQL flagged the `<script|iframe|object|embed>` regex in sanitizeCss as "incomplete multi-character sanitization" (high severity x2 — src + dist). A single-pass replace is bypassable by nested patterns like `<scr<script>ipt>`, which collapse to `<script>` after one substitution. Fix: strip ALL HTML tags from the rich-CSS payload via sanitize-html with allowedTags: [] (the library iterates internally and is not bypassable). The CSS-function regexes for `expression(...)` and `url(javascript:...)` remain — they target CSS syntax, not HTML, and CodeQL did not flag them. Verified: - 18/18 unit tests pass (no regression) - Sanity script confirms `<scr<script>ipt>` no longer leaves a `<script` substring in the output - Flagged regex literal absent from rebuilt dist/index.js - Lint clean Resolves: CodeQL failure on PR #85
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Security Bundle: APS-19076 / APS-19077 / APS-19078
This PR resolves three security findings on
browserstack/github-actionsin a single bundled change. All three target distinct files in the same repo and were approved as a single PR by the assignee.setup-envenv-var injectionbrowserstack-report-actionstored XSSAPS-19076 — env-var injection from rerun API (CVSS 9.3)
Issue.
setBStackRerunEnvVars()insetup-env/src/actionInput/index.jsdidObject.keys(variables).forEach((k) => core.exportVariable(k, variables[k]))over the BrowserStack rerun API response with no allowlist. A caller who could influence the API response (compromised account, MITM, malicious server-side state) could inject arbitrary env vars into the workflow runner —NODE_OPTIONS=--require /tmp/evil.js,PATH=/tmp/evil:/usr/bin,GITHUB_TOKEN=...overrides — leading to RCE / token exfiltration.Fix.
ALLOWED_RERUN_ENV_VARS = ['BROWSERSTACK_RERUN', 'BROWSERSTACK_RERUN_TESTS', 'BROWSERSTACK_BUILD_NAME']tosetup-env/config/constants.js.core.exportVariableis now called only for allowlisted keys; everything else triggerscore.warning(...)so non-allowlisted names surface in the runner log without silently failing.setup-env/dist/index.js(committed bundle is what the action runs).Reviewer note. If the rerun API legitimately sets additional names in production, the list will need to grow — they will be visible as
core.warninglines in workflow logs. Conservative by design.APS-19077 — workflow supply chain + token scope (CVSS 8.7)
Issue.
.github/workflows/setup-env.ymlandsetup-local.ymlusedactions/setup-node@master(floating tag — anyone with push access to setup-node could inject malicious code into our CI) and had nopermissions:block (defaultGITHUB_TOKENhad write scope it didn't need).Fix.
actions/checkout@v4->actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2.actions/setup-node@master->actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4.4.0.permissions: { contents: read }to both files (these workflows only run unit tests; they do not push, comment, or release).APS-19078 — report HTML stored XSS (CVSS 7.6)
Issue.
browserstack-report-action'sReportProcessor.processReport()wrotebasicHtml/richHtml/richCssfrom the BrowserStack reporting backend directly intosummary.addRaw(...)(renders in the GitHub Actions summary UI) and the artifact HTML file, with only smart-quote and<tbody>transforms — no sanitization. A malicious build name, test title, or test output could embed<script>/onerror/javascript:URLs and execute JS in the GitHub UI context (CSRF the user, exfiltrate session cookies).Fix.
sanitize-html ^2.17.3tobrowserstack-report-action/package.json+ lockfile.ReportProcessor.js: definedHTML_SANITIZE_OPTIONS(allowed tags/attributes;disallowedTagsMode: 'discard'; no protocol-relative; onlyhttp/https/data/mailtoschemes); wrapped bothbasicHtmlandrichHtmlwithsanitizeHtml(...). DefinedsanitizeCss(css)that stripsexpression(...),url(javascript:...)and any<script|iframe|object|embed>tags fromrichCss.UploadFileForArtifact.js: defined CSP metadefault-src 'none'; style-src 'unsafe-inline'; img-src 'self' data: https:; font-src data:; script-src 'none';injectCspMeta(html)inserts it into<head>(or constructs a head if missing) beforefs.writeFileSync. Defense-in-depth on top of the sanitizer.browserstack-report-action/dist/index.js.Testing
setup-env/setup-env/browserstack-report-action/browserstack-report-action/setup-env: rewrote pre-existing testSets environment variables from BrowserStack API response(was usingVAR1/VAR2which would now be rejected) to use the three allowlisted names; added negative-case testRejects non-allowlisted env vars from BrowserStack API response (APS-19076)that assertsNODE_OPTIONS/PATH/GITHUB_TOKENare NOT exported and thatcore.warningis called for each.Manual XSS strip sanity — exercised the same
HTML_SANITIZE_OPTIONSused byReportProcessor.jsagainst six payloads:<img src=x onerror=alert(1)><img src="x" /><script>alert(1)</script>(empty)<a href="javascript:alert(1)">click</a><a>click</a><svg onload=alert(1)></svg>(empty)<iframe src="javascript:alert(1)"></iframe>(empty)<p>safe <b>content</b></p>No BrowserStack live session run — this is a GitHub Actions library, not a session/SDK runtime; the actions only set env vars and start the BrowserStackLocal binary, so the
bs-sessionskill does not apply.Integration test evidence
Both fixes were exercised end-to-end against the built
dist/index.jsbundles (not just the source) by stubbing the BrowserStack reporting backend on127.0.0.1and capturing the action's outputs (GITHUB_ENV,step-summary.md, artifact HTML).data:-on-<a>bypass)Test A — APS-19076 env-var allowlist (15 assertions, all pass):
~/source/aps-investigations/tickets/APS-19076-19077-19078/repros/aps-19076-test-a-allowlist.jsGITHUB_ENV: recorded in~/source/aps-investigations/tickets/APS-19076-19077-19078/results/APS-19076-test-a-allowlist-20260507-152548.jsonundergithubEnvContents— confirmsBROWSERSTACK_RERUN/BROWSERSTACK_RERUN_TESTS/BROWSERSTACK_BUILD_NAMEare exported andNODE_OPTIONS/PATH/GITHUB_TOKEN/etc. are dropped withcore.warninglines.Test B — APS-19078 sanitize + CSP (25 assertions, all pass):
~/source/aps-investigations/tickets/APS-19076-19077-19078/repros/aps-19078-test-b-sanitize-csp.js~/source/aps-investigations/tickets/APS-19076-19077-19078/results/APS-19078-test-b-summary-20260507-153512.md~/source/aps-investigations/tickets/APS-19076-19077-19078/results/APS-19078-test-b-artifact-20260507-153512.html~/source/aps-investigations/tickets/APS-19076-19077-19078/results/APS-19078-test-b-sanitize-csp-20260507-153512.jsondata:-URL bypass caught by integration testing — the first dist-level run of Test B failed one assertion:<a href="data:text/html,<script>alert(6)</script>">data-click</a>survived sanitization in both the summary and artifact, becauseallowedSchemespermitteddata:on every element. Folded into this PR as commit67f66ea—allowedSchemesreduced to['http', 'https', 'mailto']andallowedSchemesByTag: { img: ['http', 'https', 'data'] }added so legitimate inline<img src="data:image/png;base64,...">screenshots still render. Test B was extended with a positive assertion that the safe<img data:base64>survives — passes after the fix.Jira tickets
Checklist